PR #44 Review — Chart accessibility, bUnit, and leak fixes

Repo: syncfusion/blazor-toolkit Branch: 1047330-charts → main Diff: +385 / −120 across 20 files Author: @Yokesh-SF4393 Status: Open — 1 requested changes (@Sittiq3586), 2 older approvals dismissed

Summary

This PR addresses three categories of bugs in the Blazor Toolkit Chart component:

1. Invalid ARIA roles propagated to the DOM ("count", "Count" — not valid WAI-ARIA roles)
2. Decorative SVG nodes leaking focus + unlabeled-image announcements to assistive technology
3. Process-wide static font-measurement cache causing memory leaks on long-lived Interactive Server hosts

The fix introduces a central role validator, makes decorative SVG attributes conditional, and moves font caches to an instance-scoped lifetime on SfChart.

Verification Against Stated Root Causes

Stated root cause Fix landed? Notes
Sample count/Count (invalid ARIA role) ✅ Validated at setter on six types; samples and tests now use "status" / "heading"
SvgRect/SvgPath unconditionally rendered tabindex="" and role="img" ✅ New EffectiveTabIndex / EffectiveRole suppress attributes when decorative
Tooltip created without ARIA live region ⚠️ Partial Description in the PR body mentions role="status" + aria-live + MutationObserver in chart.js, but no JS files are modified in this PR
Static SizePerCharacter/ChartFontKeys leaks in Interactive Server ✅ Moved to per-instance ConcurrentDictionary on JsInteropState; cleared in DisposeAsyncCore
DisposeAsyncCore overridden only to call base ✅ Override removed; cache clear added in lifecycle partial

Code-Review Findings

1. ✅ ARIA-role validator (src/Components/Charts/Common/Utils/Helper.cs)

Centralized in DataVizCommonHelper.AriaRoleValidator with the WAI-ARIA 1.2 abstract role set and case-insensitive comparison. Applied via setter across:

• ChartAnnotations.cs
• ChartSubTitleStyle.cs
• ChartTitleStyle.cs
• LegendSettings.cs
• ChartSeries.cs
• ChartTrendline.cs
• SfChart.razor.Members.cs

This is the right shape — fail-fast at component init with a clear ArgumentException and helpful URL.

Minor nit: Helper.cs ends without a trailing newline (\ No newline at end of file). Please add one and check other touched files for the same; some SDKs warn on it.

2. ⚠️ Decorative SVG attributes — close, but asymmetric

SvgRect.razor.cs suppresses role/tabindex when AriaHidden == "true".

SvgPath.razor.cs suppresses only when AccessibilityText is empty — not when AriaHidden == "true".

role="@(string.IsNullOrEmpty(EffectiveRole) ? null : (object)EffectiveRole)"

This means a path with AriaHidden="true" and no AccessibilityText will still emit role="img" because EffectiveRole only checks AccessibilityText. Recommend:

private string EffectiveRole =>
    (string.Equals(AriaHidden, "true", StringComparison.OrdinalIgnoreCase)
     || string.IsNullOrEmpty(AccessibilityText))
    ? string.Empty : "img";

Symmetry between SvgRect and SvgPath (both gated by AriaHidden OR no accessible name) is desirable — the PR description lumps them together, but the implementation differs.

Also note: tabindex="0" (default empty TabIndex) was the prior bug. string.IsNullOrEmpty(EffectiveTabIndex) returning null correctly omits the attribute in Razor — but only because the cast is (object). Worth a unit test asserting the attribute is absent from the rendered HTML, not just empty-valued.

3. ⚠️ Font cache leak fix — incomplete per reviewer

The reviewer raised two valid concerns, partially addressed:

a. Non-SfChart callers still take a fallback path. In ChartHelper.cs:

var sfChart = chart as Charts.SfChart;
if (sfChart is null)
    return GetCharSize(character, font);

Non-SfChart call sites silently fall through to the no-cache, character-by-character approximation. If any sibling chart component (SfSparkline, SfRangeNavigator, SfStockChart, augmentation components, etc.) calls this overload, they re-measure indefinitely. Either:

• Audit and migrate sibling charts to forward this, or
• Document the limitation in XML doc and add a TODO with a tracking issue.

b. ConcurrentDictionary<string, byte> _requestedFontKeys is correct, but only used inside GetCharSizeListAsync and GetDistinctCharacter. The byte value carries no semantics; consider using a strongly-typed set marker or simply a HashSet<string> held behind a lock to make intent clearer. TryAdd here is also subtly different from Add — both Add and Contains+Add patterns were replaced; keep a unit test that double-enqueues the same key and expects a no-op.

c. DisposeAsyncCore() now clears caches, but JsInteropState._fontSizeCache and _requestedFontKeys are non-null ConcurrentDictionary fields initialized inline. The Clear() call uses ?. defensively. If JsInteropState is ever constructed via new JsInteropState() after partial init (e.g., serialization), _fontSizeCache could be null. Right now it's assigned with = new(), so this is fine — keep an eye out.

4. 🐛 Currently broken test — ChartSubTitleStyle Count → heading (bUnit)

In tests/Syncfusion.Blazor.Toolkit.BUnitTest/Charts/Chart/Axis/ChartBasic.razor:

- AccessibilityRole="Count"
+ AccessibilityRole="heading"

The bUnit test was updated to "heading" while the Playwright sample was updated to "Status" ("status" after validator lower-casing). Casings now diverge between sample and test — flagged explicitly by the reviewer. Recommend a unified helper or shared constant.

Also: there is no negative-path test that asserts ArgumentException is thrown when an invalid role is set on any of the six types. E.g.:

Assert.Throws<ArgumentException>(
    () => ctx.RenderComponent<SfChart>(p => p.Add(x => x.AccessibilityRole, "count")));

Adding these is essential to prevent regressions of the original bug — the validator is the entire safety net for finding #1.

5. 🧹 ChartAxisRenderer.cs — bundling unrelated fix

The two-line change:

- option.StrokeWidth = Axis?.Renderer?.MajorGridLinesWidth ?? 0;
+ option.StrokeWidth = Axis?.MajorGridLines.Width ?? 0;

is the right shape (fixes null/incorrect grid width), but it's unrelated to accessibility/leak fixes. Reviewer flagged "Bundled ChartAxisRenderer rendering fix should be a separate PR". Agree — keep PRs atomic; it complicates bisect and cherry-pick into patch releases.

6. 📝 XML doc / SDK impact

• Three <remarks> blocks now exceed style guides in some files (long inline comments). Verify dotnet build /p:TreatWarningsAsErrors=true is green if the SDK is configured that way.
• The XML cref list in Helper.cs references types in other files (SfChart, etc.). Cross-file internal cref resolution typically requires InternalsVisibleTo on the docs assembly or that the cref be reachable. Confirm CI builds docs cleanly.
• Removing protected override ValueTask DisposeAsyncCore() from SfChart.razor.cs was correct; ensure nothing relied on dispose order (none seen in diff). The lifecycle now lives entirely in the partial SfChart.razor.LifeCycle.cs.

7. 🟡 Missing from the diff (vs. description)

• chart.js tooltip role="status" / aria-live / MutationObserver — described in the PR body as one of the five solution items but no JavaScript file is modified. The "First-to-tooltip narration race" reviewer concern is therefore unresolved in this PR. Either land the JS change in this PR or re-scope the description.
• No release notes added in RELEASE.md / changelog (typical for Syncfusion). The PR says "No breaking changes", but switching AccessibilityRole from a free-string auto-prop to a validating one is a behavior change: existing apps using "count" will now throw ArgumentException at render. Confirm with PM whether this is acceptable pre-release vs. needs a softer landing (warning first, throw in the next major).

8. 🔍 Casing / RTL / fallback consistency

The MeasureText(string, ChartFontOptions, object chart) overload:

var sfChart = chart as Charts.SfChart;
if (sfChart is not null && sfChart._fontSizeCache.TryGetValue(key, out Size? value))
{
    charSize = value;
    return new Size(charSize.Width * (fontSize / 100), charSize.Height * (fontSize / 100));
}

When chart is not an SfChart, the function falls through to the non-RTL loop without caching. This means RTL measurements on non-SfChart callers will be re-measured (or worse, computed via the no-cache GetCharSize(character, font) path with a default fallback width of 50 px). If any non-SfChart consumer (datasets, legend rendering, annotation rendering pipeline, etc.) calls this code path with RTL text, results may be inconsistent with the SfChart path. Worth a follow-up audit or at minimum an explicit comment.

Strengths

• ✅ Solid accessibility posture: fail-fast validation at the parameter setter is the correct pattern. Decorative-node suppression is the right primitive.
• ✅ Instance scoping of caches is the textbook fix for the Interactive Server leak.
• ✅ Casing-correct role set uses StringComparer.OrdinalIgnoreCase consistently.
• ✅ Removal of redundant override + clean docs help the next maintainer.
• ✅ Samples and most tests updated (Annotation.razor, ChartBasics.razor, Annotation.razor test).

Verdict

Approve after these are addressed (or disagreements noted):

1. Required before merge

• Land the chart.js tooltip ARIA-live fix in this PR, or scope the description to exclude it and open a follow-up (currently the PR claims a fix that isn't in the diff).
• Add negative-path ArgumentException unit tests for invalid ARIA roles on all six types.
• Align test vs. sample role casing (heading vs. status) or share via a constant.

2. Strongly recommended

• Make SvgPath.EffectiveRole gate on AriaHidden == "true" || AccessibilityText empty, matching SvgRect.
• Audit all non-SfChart callers of MeasureText/GetDistinctCharacter, or document the limitation with a tracking issue.
• Move ChartAxisRenderer.cs change to its own PR.

3. Nice to have

• Confirm dotnet build /p:TreatWarningsAsErrors=true is clean.
• Add trailing newline to Helper.cs (and verify other edited files).
• Promote the role validator to a public/internal type that other toolkit components can reuse.

4. Behavior-change callout

The validator changes the public-facing surface contract. Coordinate with docs/release notes and confirm with PM before tagging a release. Consider landing as a warning for one release cycle before promoting to ArgumentException.

Note: This file was generated as a code-review artifact. The MCP my-mcp-server only points at https://gitea.syncfusion.com, which doesn't host the GitHub repo, so the comment couldn't be auto-posted. Copy the relevant sections into the GitHub PR thread manually.

Review changes:

1. Tooltip live-region changes are not part of this PR; the description has been updated to reflect the actual scope of the implemented changes.
2. count is not a valid WAI-ARIA role and has been replaced with valid roles such as heading and status as part of this fix. Role validation is already enforced centrally through EnsureValidRole(), which throws an ArgumentException for unsupported roles. Existing tests have been updated accordingly and are passing successfully. Therefore, no additional issue was identified during validation.
3. status and heading are intentionally used in different chart component scenarios after replacing the previously invalid count role. Both values are valid WAI-ARIA roles and the validation logic is role-agnostic. The Annotation sample and its corresponding test are already aligned (status), so no functional inconsistency exists and no additional change is required.
4. SvgPath does not expose an AriaHidden parameter. The current change intentionally suppresses accessibility attributes when AccessibilityText is empty, which addresses the reported unlabeled-image accessibility issue. No SvgPath scenario requiring aria-hidden was identified in the current implementation.
5. Audited the current call sites for MeasureText() and GetDistinctCharacter(). No non-SfChart consumers were identified, and all usages are within the chart rendering pipeline. The instance-scoped cache therefore covers the current usage paths, so no additional action is required.
6. The ChartAxisRenderer.cs change was identified and fixed while validating the chart-related issues addressed in this PR. Since the change is small, low-risk, and part of the same chart validation effort, I have kept it in this PR rather than creating a separate follow-up PR.
7. Verified by building the solution and executing the bUnit test suite. The build completed successfully, and all related bUnit test cases passed without issues.
8. Added the trailing newline and verified the modified files.
9. The validator was introduced to address the chart accessibility issue and is currently scoped to the chart implementation. No additional toolkit consumers requiring this validation were identified. The current scope was intentionally kept minimal, and broader reuse can be considered when a common requirement arises.